Skip to content

Conversation

@melanietreitinger
Copy link
Contributor

Fixes #187

@melanietreitinger melanietreitinger force-pushed the issue-187-courseidforevents branch from 31120ff to 371895d Compare March 27, 2024 15:47
@NinaHerrmann
Copy link
Contributor

Hey Melanie thank you for your contribution.
Could you check why the tests are failing? :)
Could you elaborate on why you think we should change the context? I do not have a very strong opinion on it but from my perspective, the process is still in the system context 🤔

@melanietreitinger melanietreitinger force-pushed the issue-187-courseidforevents branch 6 times, most recently from 927397a to e3b36ad Compare April 4, 2024 07:44
@melanietreitinger
Copy link
Contributor Author

Hi Nina,

changing the context from 'system' to 'course' was necessary when using the log table field courseid - otherwise the tests failed with 'Inconsistent courseid - context combination detected'.

I fixed the tests - sorry for needing so many attempts... 🙈

@justusdieckmann
Copy link
Contributor

Hey Melanie,

thank you very much! Nothing to feel sorry about :) I think maybe it would be nicer to not store the context in the process object, but instead create it in the event_from_process function just when needed. If I remember correctly, context objects are cached pretty heavily, so even instancing a context_course object once for every step in a workflow shouldn't cause much overhead

@melanietreitinger
Copy link
Contributor Author

Hi @justusdieckmann,

I tried that in the first place, but it didn't work because when a course is deleted, the context is deleted, too, and it can't be re-created because the course is gone... 😢

@melanietreitinger melanietreitinger force-pushed the issue-187-courseidforevents branch from e3b36ad to 4625d80 Compare July 3, 2024 07:22
@bluetom bluetom merged commit eea82d4 into learnweb:master Mar 23, 2025
@melanietreitinger melanietreitinger deleted the issue-187-courseidforevents branch March 27, 2025 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improvement: Better events

4 participants